Skip to content

feat(scheduling): backend-driven HCP placement - #6657

Open
Chai-bot (redhat-chai-bot) wants to merge 2 commits into
Azure:mainfrom
redhat-chai-bot:hcp-scheduling-phases01
Open

feat(scheduling): backend-driven HCP placement#6657
Chai-bot (redhat-chai-bot) wants to merge 2 commits into
Azure:mainfrom
redhat-chai-bot:hcp-scheduling-phases01

Conversation

@redhat-chai-bot

@redhat-chai-bot Chai-bot (redhat-chai-bot) commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Implements the RP-backend-driven HCP scheduling design from #6649, delivered as one commit per phase.

Commit 1 — Phase 0: Scheduling Data Foundation (scheduling-phase0-data-foundation.md)

Pure data plumbing, no scheduling logic or new controllers.

  • Add ReadyResourceIDs []string and NotReadyResourceIDs []string to ManagementClusterScheduling.Status (fleetapi) + regenerated deepcopy.
  • The fleet CapacityReportingController now mirrors both lists from CapacityReport.Status.HostedControlPlanes into the scheduling doc, in the same read-modify-write path that already mirrors capacity/usage/requests.
  • Unit test for the mirroring.

Commit 2 — Phase 1: Swift-NIC Scheduling + Provision Shard Pinning (scheduling-phase1-swift-nic-scheduling.md)

Moves HCP placement into the backend using a swift-NIC capacity strategy (each HCP consumes 3 swift-NICs; conservative, never overbooks).

Capacity formula (per eligible MC):

available = ScaleCeiling.Capacity["aro.openshift.io/swift-nic"]
          - ObservedResources.Usage["aro.openshift.io/swift-nic"]
          - countNonEmpty(NotReadyResourceIDs) * 3
          - countNonNil(PendingAssignedClusters) * 3
fit if available >= 3

(nil/empty not-ready and pending entries reserve no capacity)

  • API: ServiceProviderCluster.Spec.ManagementClusterResourceID (scheduler intent; distinct from CS-confirmed Status) and ManagementClusterScheduling.Status.PendingAssignedClusters + regenerated deepcopy.
  • New PlacementController (cluster-keyed, single worker): eligible-MC filter (Schedulable + Ready), computes available swift-NICs per MC, rejects < 3, and spreads — selects the eligible MC with the highest available capacity (distribute load), tie-break by lowest resource ID. Pure, unit-tested selection. Rollout backfill: Spec nil + Status set → backfill Spec = Status (no reschedule). Two-step conflict-retry write: reserve in PendingAssignedClusters, then set Spec. Transient failures return an error and let the workqueue retry with backoff.
  • CapacityReportingController: observation-based pending cleanup — drops PendingAssignedClusters entries that now appear in ReadyResourceIDs ∪ NotReadyResourceIDs, in the same write.
  • New PendingCleanupController (MC-keyed periodic sweep): removes stale pending entries (SPC points elsewhere / SPC deleted); keeps entries still pointing here or with placement in progress. Uses the SPC lister/cache (not direct DB gets); favors Status over Spec for the effective MC when Status is set. Transient failures return an error for workqueue backoff.
  • ManagementClusterPlacementSync: resolves Status from CS only until a shard is observed — once Status.ManagementClusterResourceID is set, the CS lookup is skipped. On Spec != Status (both set) it logs the drift only and does not mutate Spec (no self-heal); reconciling Status over Spec is left to PendingCleanupController. (Drift would only occur if CS ignored the pin or changed placement post-commit — surfaced via logs/metric, not silently corrected.)
  • Creation pipeline: ClusterPendingClusterServiceIDAssign gated on Spec.ManagementClusterResourceID != nil; ClusterClusterServiceCreate pins the placed MC's provision shard via ClusterBuilder.ProvisionShardID(...) (OCM SDK method, not a property map).
  • Tabular unit tests across placement selection (spread), formula edge cases (incl. nil pending entries), both cleanup paths, rollout backfill, drift log-only detection, the gating precondition, and shard pinning.

Observability

This PR adds backend metrics, an alert, and CI visualization panels:

  • Metric rename: the placement-state gauge is backend_cluster_placement_state{state="placed|unplaced|mismatch"} (renamed from backend_serviceprovidercluster_placement_state), recomputed each tick from the ServiceProviderCluster cache. mismatch = Spec set AND Status set AND Spec != Status — the drift signal that replaces self-heal.
  • New histogram: backend_cluster_placement_delay_seconds measures creation→placement latency (time.Since(cluster.SystemData.CreatedAt)), observed once per fresh placement.
  • Alert: SchedulerPlacementMismatch (observability/alerts/scheduler-placement-prometheusRule.yaml, with a promtool test; registered in alerts-rp-services.yaml and the generated bicep regenerated) fires when backend_cluster_placement_state{state="mismatch"} > 0 for 15m (warning).
  • CI visualization panels: two panels added to test/cmd/aro-hcp-tests/gather-observability/queries.yaml — a placement-delay percentile chart (p50/p90/p99 over backend_cluster_placement_delay_seconds_bucket) and a stacked placement-state chart by state.

These observability additions are code/config within this PR (Prometheus rule + gather-observability query panels); there are no Grafana dashboard changes and no dashboard screenshots to attach.

Validation

make verify-deepcopy (coreapi + fleetapi) clean · go build internal + backend + fleet · go test for all touched packages (incl. metrics, placement, prometheus-rules, gather-observability) · make lint → 0 issues · promtool test on the alert rule.

Design reference: #6649. Phase 2 (multi-resource HCPResourceRequirements scheduling) is intentionally out of scope.


AI-generated PR. Please review for accuracy.

…o management cluster scheduling doc

Phase 0 of HCP scheduling: pure data plumbing, no scheduling logic and no new
controllers.

- fleetapi: add ManagementClusterScheduling.Status.ReadyResourceIDs and
  NotReadyResourceIDs ([]string), mirrored verbatim from the CapacityReport CR's
  Status.HostedControlPlanes.{ReadyResourceIDs,NotReadyResourceIDs}. Regenerated
  deepcopy.
- fleet: CapacityReportingController now mirrors those two lists onto the
  ManagementClusterScheduling document using the same read-modify-write path that
  already mirrors capacity/usage/requests into ObservedResources.
- test: assert the controller mirrors ReadyResourceIDs/NotReadyResourceIDs from
  the CapacityReport source onto Status.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 24, 2026 14:00
@openshift-ci

openshift-ci Bot commented Aug 24, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: redhat-chai-bot
Once this PR has been reviewed and has the lgtm label, please assign roivaz for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements backend-driven initial HCP placement by introducing scheduler intent on ServiceProviderCluster.Spec, mirroring observed readiness sets into the fleet scheduling document, and pinning Cluster Service provisioning to the scheduler-selected management cluster.

Changes:

  • Extend fleet scheduling status to include ready/not-ready HCP resource ID sets plus pending placement reservations.
  • Add backend placement + pending-reservation cleanup controllers, and make placement sync self-heal Spec to match Cluster Service observed reality.
  • Gate Cluster Service ID assignment on placement intent and pin Cluster Service provision shard during cluster creation.

Reviewed changes

Copilot reviewed 15 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
internal/api/fleetapi/zz_generated.deepcopy.go Regenerated deepcopy for new scheduling status fields.
internal/api/fleetapi/types_management_cluster_scheduling.go Adds Ready/NotReady resource ID sets and pending assignment reservations to scheduling status.
internal/api/coreapi/zz_generated.deepcopy.go Regenerated deepcopy for new SPC spec field.
internal/api/coreapi/types_serviceprovider_cluster.go Adds Spec.ManagementClusterResourceID (scheduler intent) to SPC.
fleet/pkg/controllers/capacityreporting/controller_test.go Adds unit tests for mirroring ready/not-ready IDs and dropping observed pending reservations.
fleet/pkg/controllers/capacityreporting/capacity_reporting_controller.go Mirrors ready/not-ready IDs into scheduling doc and cleans up observed pending reservations.
backend/pkg/controllers/cluster/placement/placement_controller.go New controller to select an eligible MC by swift-NIC capacity and write placement intent + reserve capacity.
backend/pkg/controllers/cluster/placement/placement_controller_test.go Unit tests for capacity math, candidate selection, and placement/backfill behavior.
backend/pkg/controllers/cluster/placement/pending_cleanup_controller.go New controller to periodically remove stale pending placement reservations.
backend/pkg/controllers/cluster/placement/pending_cleanup_controller_test.go Unit tests for stale pending reservation cleanup behavior.
backend/pkg/controllers/cluster/placement/management_cluster_placement_sync.go Always reconciles observed placement from Cluster Service and self-heals Spec on drift.
backend/pkg/controllers/cluster/placement/management_cluster_placement_sync_test.go Updates tests for always-reconcile + spec self-heal semantics.
backend/pkg/controllers/cluster/creation/cluster_pending_cluster_service_id_assign_controller.go Gates PendingClusterServiceID assignment on placement intent being resolved in SPC.Spec.
backend/pkg/controllers/cluster/creation/cluster_pending_cluster_service_id_assign_controller_test.go Tests new placement gating behavior for PendingClusterServiceID assignment.
backend/pkg/controllers/cluster/creation/cluster_cluster_service_create_controller.go Resolves and pins provision shard based on scheduler-selected management cluster.
backend/pkg/controllers/cluster/creation/cluster_cluster_service_create_controller_test.go Tests provision shard pinning derived from placed management cluster.
backend/pkg/app/backend.go Wires up PlacementController and PendingCleanupController and passes MC lister into CS create controller.
Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/api/coreapi/types_serviceprovider_cluster.go Outdated
Comment thread internal/api/coreapi/types_serviceprovider_cluster.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file

Comment thread backend/pkg/controllers/cluster/placement/placement_controller.go
Comment thread backend/pkg/controllers/cluster/placement/management_cluster_placement_sync.go Outdated
Comment thread backend/pkg/controllers/cluster/placement/pending_cleanup_controller.go Outdated
Comment thread backend/pkg/controllers/cluster/placement/placement_controller.go Outdated
Comment thread docs/cosmos-data-flow.md Outdated
Comment thread internal/api/coreapi/types_serviceprovider_cluster.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file

Comment thread backend/pkg/controllers/cluster/placement/management_cluster_placement_sync.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 20 changed files in this pull request and generated 2 comments.

Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)

docs/cosmos-data-flow.md:998

  • The PendingCleanupController docs table lists only Spec.ManagementClusterResourceID as read from ServiceProviderCluster, but the controller also reads Status.ManagementClusterResourceID and prefers it when set. The documented read set should include both fields to match the actual Cosmos contract.
| Read | `ServiceProviderCluster` (per pending entry) | <ul><li>`Spec.ManagementClusterResourceID`</li></ul> |

Comment thread docs/cosmos-data-flow.md Outdated
Comment thread docs/cosmos-data-flow.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file

Comment thread backend/pkg/controllers/metrics/placement_metrics_controller.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 25 changed files in this pull request and generated 2 comments.

Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file

Comment thread backend/pkg/controllers/cluster/placement/placement_controller.go
Comment thread docs/cosmos-data-flow.md Outdated
Copilot AI review requested due to automatic review settings August 25, 2026 09:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 25 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file

Comment thread backend/pkg/controllers/metrics/placement_metrics_controller.go Outdated
Comment thread backend/pkg/controllers/metrics/placement_metrics_controller.go Outdated

@deads2k David Eads (deads2k) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs heavy changes.

Comment thread backend/pkg/app/backend.go Outdated
Comment on lines +1149 to +1150
go placementController.Run(ctx, 1) // single worker: capacity selection reads/reserves across MCs and must not race itself
go pendingCleanupController.Run(ctx, 1) // single worker: sweeps pending reservations per management cluster

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

design problem if this needed. Since we have optimistic write enforcement AND we write the pending and actual assignments to the same place as estimated usage AND we write estimated usage, we should be able to run in parallel without an issue and assignments to differnet management clusters should be non-conflicting.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right. we can safely have multiple workers

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 9a34ddd — the Placement controller now runs with multiple workers (removed the single-worker restriction).


AI-generated. Review for accuracy.


AI-generated. Review for accuracy.

@@ -235,10 +239,18 @@ func (c *clusterClusterServiceCreateSyncer) csClustersMatchingClusterByAzureInfo
func (c *clusterClusterServiceCreateSyncer) createClusterServiceCluster(ctx context.Context, cluster *coreapi.HCPOpenShiftCluster, serviceProviderCluster *coreapi.ServiceProviderCluster, tenantID string) (*arohcpv1alpha1.Cluster, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before calling createClusterServiceCluster, ensure that serviceProviderCluster.Spec.ManagementClusterResourceID is non-nil and print a message if it is nil and then return nil.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provisionShardID() handles unplaced clusters but returns an err which in turns add the item back to the queue. as you said we should return nil. the update on the SPC once placement is available, will wake us up again.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 9a34ddd — before creating the CS cluster, if serviceProviderCluster.Spec.ManagementClusterResourceID is nil we now log and return nil (no error, no requeue); the SPC update when placement lands re-triggers. provisionShardID no longer errors on an unplaced cluster.


AI-generated. Review for accuracy.


AI-generated. Review for accuracy.

len(cluster.ServiceProviderProperties.ClusterServiceID.String()) == 0)
len(cluster.ServiceProviderProperties.ClusterServiceID.String()) == 0) &&
serviceProviderCluster != nil &&
serviceProviderCluster.Spec.ManagementClusterResourceID != nil

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required here. We can determine the future ID and unleash all sorts of Azure changes before we assign a management cluster.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is me being pesimistic. why would we unleash all sorts of azure changes before we know we get a slot to schedule

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this unchanged for now — it's an open design question (gate CS-ID assignment on placement vs. determine the future ID before assigning a management cluster). The design author is currently defending the gating on this thread, so I'll hold any change until that's settled; happy to drop the gating if that's the conclusion.


AI-generated. Review for accuracy.


AI-generated. Review for accuracy.

}
if !c.needsWork(cachedSPC) {
logger.V(1).Info("ServiceProviderCluster already has ManagementClusterResourceID, skipping")
if cachedSPC.Status.ManagementClusterResourceID != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change looks strictly worse. Put needswork back.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved by reverting the whole file to upstream/main in 9a34ddd — the needsWork/original behavior is back; none of the changes flagged here remain.


AI-generated. Review for accuracy.


AI-generated. Review for accuracy.

cachedCluster, err := c.clusterLister.Get(ctx, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName)
// Status not yet observed in the cache: a write may follow, so fall back to a
// live Cosmos read to act on fresh data.
spcCRUD := c.cosmosClient.ServiceProviderClusters(key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, don't. Use a lister like we had it before.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved by the full revert of this file to upstream/main in 9a34ddd — the original lister-based approach is restored.


AI-generated. Review for accuracy.


AI-generated. Review for accuracy.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes to this file look confused. What are you trying to do. You shouldn't have to touch this at all.

@geoberle Gerd Oberlechner (geoberle) Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes in this file are probably due to the introduction of spec.ManagementClusterResourceID
the spec one is the intend and the status one is the observation.

i had a sentence in the prompt that we should check if the intended placement was honored or if status would end up showing a different mgmt cluster. and the defensive work around status ending up different than spec is probably not needed

lets revert all changes in this file

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 9a34ddd — reverted this entire file (and its _test.go) back to upstream/main; git diff upstream/main -- <this file> is now empty. The changes here were an artifact of the earlier drift-detection approach, which has been dropped. The backend.go call site was updated to match upstream's constructor signature.


AI-generated. Review for accuracy.


AI-generated. Review for accuracy.

}

// Live read to confirm work is still needed and to decide backfill vs. fresh.
spcCRUD := c.cosmosClient.ServiceProviderClusters(key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no live reads. Always use the cache

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 9a34ddd — removed the live Cosmos read; SyncOnce now decides entirely from the informer/lister cache (the write path remains read-modify-write).


AI-generated. Review for accuracy.


AI-generated. Review for accuracy.

return nil
}

// Rollout backfill: when the HCP was already placed by

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every old record will reach here. Old records need ot have this filled in based on the cluster-service cluster.

@geoberle Gerd Oberlechner (geoberle) Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. so downstream CS get to find if there is a placement already

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 9a34ddd — old records (Spec nil) now backfill Spec from the cached Status.ManagementClusterResourceID (the CS-observed placement) instead of fresh-scheduling, so downstream CS finds the existing placement.


AI-generated. Review for accuracy.


AI-generated. Review for accuracy.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is still a race condition. a cluster gets created with a previous backend version that did not yet set spec.ManagementClusterResourceID during for placement, but spc.status.ManagementClusterResourceID is not yet filled. we would decide on a placement that is different from what CS decided in the meantime.

we can call out to CS directly with the PendingClustersServiceID if it set.

// ManagementClusterPlacementSync (Status set) but the new Spec intent is
// nil, adopt the observed placement rather than re-scheduling it.
if liveServiceProviderCluster.Status.ManagementClusterResourceID != nil {
if err := c.setSpecPlacement(ctx, key, liveServiceProviderCluster.Status.ManagementClusterResourceID); err != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is non-sensical

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked in 9a34ddd — this logic was part of the old fresh-schedule path and is gone; the controller now backfills Spec from the cached observed Status for existing records and only fresh-schedules when both are unset.


AI-generated. Review for accuracy.


AI-generated. Review for accuracy.

Comment thread backend/pkg/controllers/cluster/placement/placement_controller.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 25 changed files in this pull request and generated 2 comments.

Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file

Comment thread observability/alerts/scheduler-placement-prometheusRule.yaml Outdated
Comment thread backend/pkg/controllers/metrics/cluster_info_metrics_handler.go
Comment thread backend/pkg/controllers/metrics/cluster_info_metrics_handler.go
Comment thread test/cmd/aro-hcp-tests/gather-observability/queries.yaml Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated 3 comments.

Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file

Comment thread backend/pkg/controllers/metrics/cluster_info_metrics_handler.go Outdated
Comment thread backend/pkg/controllers/cluster/placement/placement_controller.go Outdated
Comment thread docs/cosmos-data-flow.md Outdated
Comment thread backend/pkg/app/backend.go Outdated
Comment thread backend/pkg/controllers/cluster/placement/placement_controller.go
Comment thread backend/pkg/controllers/cluster/placement/placement_controller.go Outdated
Copilot AI review requested due to automatic review settings August 26, 2026 15:05
@redhat-chai-bot
Chai-bot (redhat-chai-bot) force-pushed the hcp-scheduling-phases01 branch 2 times, most recently from 0ec7d87 to 2948965 Compare August 26, 2026 15:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Comment thread internal/api/coreapi/types_serviceprovider_cluster.go Outdated
Comment thread docs/cosmos-data-flow.md Outdated
Comment on lines +230 to +246
for i := range candidates {
candidate := candidates[i]
if candidate.resourceID == nil {
continue
}
if candidate.available < swiftNICsPerHCP {
continue
}
switch {
case chosen == nil:
chosen = &candidates[i]
case candidate.available > chosen.available:
chosen = &candidates[i]
case candidate.available == chosen.available && candidate.resourceID.String() < chosen.resourceID.String():
chosen = &candidates[i]
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets collection reasoning about why we eliminate candidates and raise that via the error so we have a chance to debug

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also it is confusing that candidate elimination happens here and in selectByCapacity.

if stampIdentifier == "" {
continue
}
scheduling, err := c.fleetDBClient.Stamps().ManagementClusters(stampIdentifier).Scheduling().Get(ctx, fleetapi.SchedulingResourceName)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the informer cache

Comment thread backend/pkg/controllers/cluster/placement/placement_controller.go
managementClusterResourceID := key.GetResourceID()
schedulingCRUD := c.fleetDBClient.Stamps().ManagementClusters(key.StampIdentifier).Scheduling()

existing, err := schedulingCRUD.Get(ctx, fleetapi.SchedulingResourceName)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the informer cache to get the scheudling document. dont use the CRUD

return nil
}

// Rollout backfill: when the HCP was already placed by

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is still a race condition. a cluster gets created with a previous backend version that did not yet set spec.ManagementClusterResourceID during for placement, but spc.status.ManagementClusterResourceID is not yet filled. we would decide on a placement that is different from what CS decided in the meantime.

we can call out to CS directly with the PendingClustersServiceID if it set.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated 2 comments.

Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file

}

func (h *clusterInfoMetricsHandler) Sync(_ context.Context, serviceProviderCluster *coreapi.ServiceProviderCluster) {
func (h *clusterInfoMetricsHandler) Sync(ctx context.Context, serviceProviderCluster *coreapi.ServiceProviderCluster) {
go externalAuthMetricsController.Run(ctx, 1)
go clusterInfoMetricsController.Run(ctx, 1)
go placementSyncController.Run(ctx, 20)
go placementController.Run(ctx, 20)
…servations, provision shard pinning

Builds on phase 0 (ready/not-ready mirroring). Adds capacity-aware placement of
HostedControlPlanes onto management clusters based on SWIFT-NIC capacity, plus
provision-shard pinning at Cluster Service creation.

Per eligible management cluster, available swift NICs are computed as:
  ScaleCeiling.Capacity[swift-nic]
    - ObservedResources.Usage[swift-nic]
    - (non-empty NotReadyResourceIDs)*3
    - (non-nil PendingAssignedClusters)*3
and an HCP fits when available >= 3 (swiftNICsPerHCP; a conservative flat
per-HCP cost that never overbooks). Nil/empty list entries do not correspond to
a real HCP and never reserve capacity.

API:
- coreapi: add ServiceProviderCluster.Spec.ManagementClusterResourceID
  (scheduler intent; drift from the observed Status placement is logged for
  investigation but NOT auto-corrected), regenerate deepcopy.
- fleetapi: add ManagementClusterScheduling.Status.PendingAssignedClusters
  (transient reservations), regenerate deepcopy.

Controllers:
- placement: new PlacementController (cluster-keyed, single worker). Backfills
  Spec from an already-observed Status placement during rollout; otherwise runs
  a pure, capacity-aware selection (fit filter + lowest-available bin-pack
  tie-break), reserves capacity in the chosen MC's PendingAssignedClusters, then
  records Spec. Transient write/conflict failures return an error so the
  workqueue retries with backoff.
- placement: new PendingCleanupController (MC-keyed periodic sweep) removes stale
  pending reservations. Effective placement favors the SPC's observed Status over
  Spec; an entry is kept when it points here or is still resolving (nil) and
  removed when it points elsewhere or the SPC is gone. SPCs are read via the
  shared lister/cache; transient failures return an error for workqueue backoff.
- placement: ManagementClusterPlacementSync reconciles Status from Cluster
  Service only while the observed placement is still unknown; once Status is set
  it skips the CS lookup. Spec/Status drift is logged, never auto-corrected.
- fleet CapacityReportingController: in the same read-modify-write, drops
  pending reservations that are now observed (Ready or NotReady).
- creation: ClusterPendingClusterServiceIDAssign gates on
  Spec.ManagementClusterResourceID != nil; ClusterClusterServiceCreate pins the
  provision shard via ClusterBuilder.ProvisionShardID resolved from the placed
  management cluster.
- wire PlacementController and PendingCleanupController into backend (single
  worker each; FleetDBClient + listers).

Tests: pure selection combos + lowest-available tie-break; capacity formula
edges incl. nil/empty entries; capacity-report pending cleanup; pending-cleanup
stale-entry matrix (Status-favored); rollout backfill; placement-sync
skip-when-Status-set and log-only drift; gated needsWork; provision shard
pinning.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.

Files not reviewed (2)
  • internal/api/coreapi/zz_generated.deepcopy.go: Generated file
  • internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)

backend/pkg/controllers/metrics/cluster_info_metrics_handler.go:62

  • Sync now takes a named ctx parameter but it is not used anywhere in the function body, which will cause a compile error (ctx declared and not used).
func (h *clusterInfoMetricsHandler) Sync(ctx context.Context, serviceProviderCluster *coreapi.ServiceProviderCluster) {

@openshift-ci

openshift-ci Bot commented Aug 26, 2026

Copy link
Copy Markdown

Chai-bot (@redhat-chai-bot): The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-parallel 8b43b8f link true /test e2e-parallel

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants